-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Continued simplification of Taxonomy model, squashed migrations [FC-0030] #33438
Continued simplification of Taxonomy model, squashed migrations [FC-0030] #33438
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Hi @bradenmacdonald!
Let me know if you need a hand to make the test pass to unblock this PR! |
@rpenido Yes, I know but I'm having some trouble with the tests actually. It seems that the LanguageTaxonomy created by a data migration is not available during the test run, and I can't figure out why. |
@rpenido OK, I figured out the problem actually. Should have this PR unblocked soon. |
e14fae3
to
fbfa001
Compare
c02da5e
to
ece987a
Compare
('content_tagging', '0003_system_defined_fixture'), | ||
('content_tagging', '0004_system_defined_org'), | ||
('content_tagging', '0005_auto_20230830_1517'), | ||
('content_tagging', '0006_simplify_models'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were starting to get problems with some of these migrations, referring to models that have been deleted or depending on migrations which depended on fixtures, which were later changed or deleted (note to self: migrations should never reference fixtures). So I've squashed the migrations here which makes it much simpler and more robust. I'd really like to get the squashed version into Quince. If possible, I'd like to squash the migrations in oel_tagging too but that would be a separate PR.
I didn't want to put more pressure than you already had! Happy that you found the issue and also squashed the migrations! Let us know if you need something! |
@pomegranited @Agrendalath Could you please review this PR? I'd like to get it merged tomorrow so it can make the Quince cutoff and since it's blocking other work; sorry for the short notice. I can ask other reviewers if your plate is full. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left 3 recommended changes and one question, but this is good to go once they've been applied.
👍
- I tested this:
- Started with the migrations in the
master
branch (content_tagging.0006_simplify_models
,oel_tagging.0010_cleanups
) - Switched to this branch and applied my suggested change (see inline comments).
- Installed the updated
openedx-learning==0.2.1
- Applied the migrations and checked the results.
- Rolled back the new data migration:
./manage.py lms migrate content_tagging 0001_squashed
- Re-applied all migrations, and checked the results again.
- Started with the migrations in the
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation -- thank you for the explanatory docstrings!
- Commit structure follows OEP-0051
# Adding taxonomy class to the language taxonomy | ||
language_taxonomy = Taxonomy.objects.get(id=-1) | ||
ContentLanguageTaxonomy = apps.get_model("content_tagging", "ContentLanguageTaxonomy") | ||
language_taxonomy.taxonomy_class = ContentLanguageTaxonomy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused.. The ContentAuthorTaxonomy
and ContentOrganizationTaxonomy
models have also been deleted, so I would expect them to cause problems just like the ContentLanguageTaxonomy
does now? No worries if they're not, I'm just curious.
openedx/core/djangoapps/content_tagging/migrations/0007_system_defined_org_2.py
Outdated
Show resolved
Hide resolved
initial = True | ||
|
||
dependencies = [ | ||
("oel_tagging", "0012_language_taxonomy"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ach.. I'm afraid this line caused an issue when I was testing openedx/openedx-learning#95 :(
Changing it to this fixes the issue, but means we need to merge/tag openedx/openedx-learning#95 first:
("oel_tagging", "0012_language_taxonomy"), | |
("oel_tagging", "0001_squashed"), |
Here's the steps I took:
- Zero-ed the migrations
oel_tagging
andcontent_tagging
, and ensured all the tables for these apps were dropped. - Checked out edx-platform
master
(b353019), and installed openedx-learningv0.2.0
- Ran migrations, which applied fine:
$ ./manage.py lms migrate Running migrations: Applying oel_tagging.0001_initial... OK Applying oel_tagging.0002_auto_20230718_2026... OK Applying oel_tagging.0003_auto_20230721_1238... OK Applying oel_tagging.0004_auto_20230723_2001... OK Applying oel_tagging.0005_language_taxonomy...Installed 185 object(s) from 1 fixture(s) OK Applying content_tagging.0001_initial... OK Applying content_tagging.0002_system_defined_taxonomies... OK Applying content_tagging.0003_system_defined_fixture... OK Applying content_tagging.0004_system_defined_org... OK Applying content_tagging.0005_auto_20230830_1517... OK Applying content_tagging.0006_simplify_models... OK Applying oel_tagging.0006_alter_objecttag_unique_together... OK Applying oel_tagging.0006_auto_20230802_1631... OK Applying oel_tagging.0007_tag_import_task_log_null_fix... OK Applying oel_tagging.0008_taxonomy_description_not_null... OK Applying oel_tagging.0009_alter_objecttag_object_id... OK Applying oel_tagging.0010_cleanups... OK
- Checked out this branch and installed the branch from Squash Tagging Migrations [FC-0030] openedx-learning#95.
- Applied migrations, which errored out:
django.db.migrations.exceptions.InconsistentMigrationHistory: Migration content_tagging.0001_squashed is applied before its dependency oel_tagging.0012_language_taxonomy on database 'default'.
- Made the above recommended change, and was able to proceed with applying the migrations:
$ ./manage.py lms migrate Running migrations: Applying oel_tagging.0011_remove_required... OK Applying oel_tagging.0012_language_taxonomy... OK Applying content_tagging.0007_system_defined_org_2... OK
- Tested rollback, which worked OK1
$ ./manage.py lms migrate content_tagging zero $ ./manage.py lms migrate oel_tagging zero
- Tested applying the squashed migrations, also worked fine:
$ ./manage.py lms migrate Running migrations: Applying oel_tagging.0001_squashed... OK Applying oel_tagging.0012_language_taxonomy... OK Applying content_tagging.0001_squashed... OK Applying content_tagging.0007_system_defined_org_2... OK
1 I always get errors about the named indexes (content_tag_taxonom_70d60b_idx
, content_tag_taxonom_b04dd1_idx
, oel_tagging_taxonom_5e948c_idx
, etc.) when rolling back migrations ("Cannot drop index 'whatever_idx': needed in a foreign key constraint"). I've seen this before these migrations were squashed, and I don't know why it happens. To get around it, I comment out the AddIndex
and AlterUniqueTogether
lines in the squashed migrations before rolling back.
Co-authored-by: Jillian <[email protected]>
d4924cd
to
8215a5e
Compare
@pomegranited Thank you for the quick and excellent review! I tried to incorporate your changes :) |
Yep, your changes look great, thank you @bradenmacdonald ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- I tested this: checked the changes, verified that the CI is passing
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository: n/a
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
This is a minor update of the content tagging app to simplify it in correspondence with openedx/openedx-learning#91
It removes the
required
field which we don't really have a use for, and it makes the language taxonomy to be managed by a migration with auto-created tags, rather than imported one time via a fixture.Testing instructions
For now there is little ability to manually test, so just make sure the tests are passing.
Deadline
None
Other information
Private-ref: FAL-3477